[FIX][14.0] connector_importer and connector_importer_product: proposition of 'fixes' for additional flexibility#141
Conversation
|
Hi @mmequignon, @simahawk, @sebalix, |
| req.update(self.work.options.mapper.get("required_keys", {})) | ||
| return req | ||
|
|
||
| return self.work.options.mapper.get("required_keys", {}) or req |
There was a problem hiding this comment.
This looks like a breaking change to me.
req = {"a": "1"}
option_req = {"b": "2"}
before, it would've returned {"a": "1", "b": "2"}
Not it returns {"b": "2"}
What's the purpose of this ?
There was a problem hiding this comment.
The purpose is to take full control of what are the required_keys if we explicitly use the parameter.
For my use case, I want to update some measurements fields over product.products where the required_keys and odoo_unique_key are barcode
It was seen during a live training session with @simahawk, but it can indeed be a subject of debate
There was a problem hiding this comment.
It's not breaking because this option is kind of new. I'm favor of this change as it makes it more clear that you have full control from the conf.
Yet, we might improve this a little bit. The original issue for this fix was that when updating records you can skip some fields in the source file. When that happens you can define a specific import.type that rewrites the required keys (eg: product name is required only on create, not on write).
Today, if you don't provide all the fields that the specific mapper defines, the record will be skipped.
We have some choices:
- remove the required key from the product mapper and rely on the fact that the system will still fail if you don't provide a name but it will happen only at creation -> issue on write solved
If we do so, then we should improve how required fields are displayed on the docs of the recordset: there we use only the mapper configuration but we could in fact retrieve required fields from the model itself by checking fields_get (we do this already in the dynamic mapper but only for technical reasons).
The drawback of this change is that for existing installations it will give a different result: the import will fail instead of report a missing key. I think we can live w/ this. Then, in the case of the product, we can document in the specific module that if you don't specify the key in the import.type it won't be skipped.
But we have opt 3, see below.
- improve this configuration by specifying if the required keys are for create or write or both.
Eg:
# for both
required_keys:
foo: __foo
# for create
for_create:
bar: bar
for_write:
baz: bazthen we can propagate the current action via ctx key (note that on the base importer we already have the create= key but is not used yet).
- by default make al required keys mandatory (as mentioned above by checking fields_get) unless you specified otherwise. We could have a specific key on the mapper, eg:
mapper:
required_keys_ignore_model_required: trueor
mapper:
required_keys:
_ignore_model_required: trueAnd to get back to the original remark by MMQ, we can have a specific flag to decide to override or not, eg:
mapper:
required_keys:
_override_mapper: trueFor the reason explained above IMO this key should be set to true by default to true because the record will fail anyway.
There was a problem hiding this comment.
I modified my commit in order to add the property required_keys_ignore_model_required into "importer.base.mapper"
|
Can you please rename your commits ? However, you can add as many lines after and describe with more technical details about what's in the commit. |
66eeb6c to
6ed2b3b
Compare
|
Thanks for you review @mmequignon I added some additional infos answering your first comment and updated the commits |
0e5d562 to
0962ab2
Compare
0962ab2 to
5409d70
Compare
Early return if no product.attribute.value is to be imported
9811540 to
d4c0377
Compare
connector_importer_product/components/product_product/record_handler.py
Outdated
Show resolved
Hide resolved
When updating product.product records, product_tmpl_id is set to False in values if not specified in the import type. But we don't want to set it to False as the value already exists in the odoo_record
d4c0377 to
fcd2242
Compare
Not forcing required default value if required_keys_ignore_mapper and required_keys are defined in options.mapper. Adding the new property required_keys_ignore_mapper in order to override the result of required_keys function
fcd2242 to
b40334a
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
|
@simahawk can we reopen this PR and eventually get it merged ? |
No description provided.